-
Notifications
You must be signed in to change notification settings - Fork 452
Fix: Multiple Windows-related issues - focus loss, CI failures, and hanging processes #2762
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
The targetCreated event handler was incorrectly switching to the first available page (pages[0]) instead of the newly created target. This caused a race condition where focus would sometimes remain on the old tab when clicking links with target="_blank" or window.open(). The fix uses the correct target ID from the event parameter (newTarget.targetInfo.targetId) instead of fetching all pages and selecting the first one. Signed-off-by: winst0niuss <chumachenko.vadym@gmail.com>
Signed-off-by: winst0niuss <chumachenko.vadym@gmail.com>
This reverts commit 2f8eadf. Signed-off-by: winst0niuss <chumachenko.vadym@gmail.com>
- Add 500ms delay to wait for authentication redirects to complete - Try direct connection to targetId from event first (optimistic path) - Fallback to openerId search if target changed during redirects - Use switchBrowserContext() for explicit tab activation - Add detailed logging for debugging failed connections The previous approach used pages[0] which did not guarantee correct tab order from CDP. This caused focus to randomly stay on the old tab when clicking links with target='_blank' that trigger multiple redirects. The new implementation: 1. Waits for redirects to stabilize (500ms) 2. Attempts direct connection to the exact targetId from the event 3. Falls back to searching by openerId if redirects changed the targetId 4. Explicitly activates the target using switchBrowserContext() This ensures reliable focus switching even with complex authentication flows involving multiple redirects. Signed-off-by: winst0niuss <chumachenko.vadym@gmail.com>
Split pages.find() call across multiple lines to comply with Biome formatting rules. Signed-off-by: winst0niuss <chumachenko.vadym@gmail.com>
This commit addresses two Windows-specific CI failures that are unrelated to PR getgauge#2762: 1. **Line endings issue**: Added .gitattributes to enforce LF line endings across all platforms. Without this, Git on Windows converts LF to CRLF (when core.autocrlf=true), causing Biome linter to fail. 2. **Test race condition**: Fixed test/unit-tests/write.test.js:214 where closeBrowser() was called without await. This caused the browser to not fully close before removeFile() attempted to delete write.html, resulting in "EBUSY: resource busy or locked" on Windows. Changes: - Add .gitattributes with "* text=auto eol=lf" to normalize line endings - Fix write.test.js after() hook to await closeBrowser() before removeFile() - Reorder operations: close browser first, then remove file Fixes Windows CI test failures in the main repository. Signed-off-by: winst0niuss <chumachenko.vadym@gmail.com>
|
@zabil Re-run tests please |
- Add targetExists() helper to verify target before connection attempts - Check target existence after redirect delay to avoid connecting to closed targets - Handle ECONNRESET gracefully as expected error during redirects - Avoid unnecessary retries on non-existent targets - Improve fallback reliability when target changes during redirects The previous implementation attempted to connect to targetId from the targetCreated event without checking if it still exists. With multiple redirects, the browser may close intermediate targets, causing ECONNRESET errors when initCRI() tries to establish a CDP connection. The new approach verifies target existence first, preventing wasted connection attempts and providing better error handling for redirect chains. Signed-off-by: winst0niuss <chumachenko.vadym@gmail.com>
Replaced manual SIGTERM/SIGKILL process killing with tree-kill library to ensure entire process tree is terminated on all platforms. This prevents orphaned Chrome renderer and GPU processes on Windows. Signed-off-by: winst0niuss <chumachenko.vadym@gmail.com>
|
Ready for tests and review |
When closing the browser, if a page has a beforeunload event handler, Taiko would crash with 'There is no handler registered for beforeunload popup'. This fix adds a flag to automatically accept beforeunload dialogs during browser closure, preventing the crash while still allowing explicit beforeunload handling during normal operation. Fixes getgauge#2752 Signed-off-by: winst0niuss <chumachenko.vadym@gmail.com>
lib/browser/launcher.js
Outdated
| const { setBrowserOptions, defaultConfig } = require("../config"); | ||
| const { eventHandler } = require("../eventBus"); | ||
| const util = require("node:util"); | ||
| const kill = require("tree-kill"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure about this library. The last update was 6 years ago
https://www.npmjs.com/package//tree-kill
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have doubts about this lib too, but currently the browser won't close on Windows (that's why we're stuck on Chromium 120.xxx).
The tree-kill package hasn't been updated in 6 years, raising maintenance and security concerns. Reverting to the original browser process termination approach using process.kill(). This reverts the process killing changes from commit 5bbafcc. Signed-off-by: winst0niuss <chumachenko.vadym@gmail.com>
|
@zabil, I've reverted the |
|
@winst0niuss Thank you for contributing to taiko. Your pull request has been labeled as a release candidate 🎉🎉. Merging this PR will trigger a release. Please bump up the version as part of this PR.Instructions to bump the version can found at CONTRIBUTING.md If the CONTRIBUTING.md file does not exist or does not include instructions about bumping up the version, please looks previous commits in git history to see what changes need to be done. |
Signed-off-by: winst0niuss <chumachenko.vadym@gmail.com>
Signed-off-by: winst0niuss <chumachenko.vadym@gmail.com>
|
@zabil , re-run tests please |
Fix beforeunload handler crash during browser closure
Signed-off-by: winst0niuss <chumachenko.vadym@gmail.com>
|
@zabil , re-run tests please |
This PR addresses multiple issues affecting Windows platform and tab management:
Issues Fixed
1. Focus loss when opening new tabs with redirects (Issue #2761)
Problem: Focus randomly remained on the original tab approximately 40-60% of the time when clicking links that open new tabs with
target="_blank"and perform multiple redirects (particularly in OAuth/SSO authentication flows).Root Cause: The implementation used
pages[0]from Chrome DevTools Protocol's target list, which didn't guarantee correct tab order and failed to account for targets changing during redirects.Solution: A two-phase approach:
targetIdfrom thetargetCreatedevent with explicit tab activationopenerIdand activates the matching targetCommits: 08328e1, 0436dd2
2. Windows CI test failures
Problem: CI tests failing on Windows due to line ending issues and test race conditions.
Solution:
.gitattributesfor line ending normalizationwrite.test.jsCommit: 95d7c07
3. Hanging Chrome processes after closeBrowser() on Windows
Problem: After running tests, Chrome renderer and GPU processes remained running on Windows, consuming system resources.
Root Cause: Standard
SIGTERM/SIGKILLsignals don't kill child processes on Windows, leaving orphaned Chrome subprocesses.Solution: Replaced manual process killing with
tree-killlibrary to ensure the entire process tree is terminated on all platforms. This prevents orphaned Chrome processes across Windows, Linux, and macOS.Commit: 79af9c9
Changed Files
lib/connection.js- targetCreated event handler improvementslib/browser/launcher.js- closeBrowser() with tree-kill.gitattributes- line ending normalizationtest/unit-tests/write.test.js- race condition fixpackage.json- added tree-kill dependency